Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add formatting and linting python in pre-commit #370

Merged
merged 7 commits into from
Sep 30, 2024

Conversation

m-fila
Copy link
Contributor

@m-fila m-fila commented Sep 17, 2024

BEGINRELEASENOTES

  • Added ruff pre-commit hook for formatting and linting python files
  • Reformatted and fixed warnings in python files
  • Added cmake targets ruff and ruff-format to lint and format python code

ENDRELEASENOTES

Linting and formatting done with ruff which should be compatible with black, flake8 and pylint while having a single configuration file. ruff is already in the stack

CI added as local hook

Selected rules ["F", "E", "W", "PLE", "PLW", "PLC"] from flake8 (pyflakes "F", pycodestyle errors "E" and warnings "W") and pylint (errors "PLE", warnings "PLW" and conventions "PLC")
I'd also consider adding some of the D1 checks for docstrings but there seems to be only a few docstrings now 🙃

The codebase seems to use mixed 2-space and 4-space indent, now the default 4-space is used in formatter

Should I also add cmake targets as in #257?

Closes #364

@m-fila
Copy link
Contributor Author

m-fila commented Sep 17, 2024

No ruff in the LCG releases? 👀

@tmadlener
Copy link
Contributor

Should I also add cmake targets as in #257?

We either adapt that to run the same checks as this, or we have to include flake8 and pylint in this PR as well. Alternatively, we close that and include the changes here.

@m-fila
Copy link
Contributor Author

m-fila commented Sep 18, 2024

Added cmake targets here so #257 can be closed if this is merged

@tmadlener
Copy link
Contributor

Very nice. Thanks :)

@jmcarcell
Copy link
Member

What about including formatting as part of Key4hepConfig.cmake? For python ruff is very fast so it won't change much, clang-format for every file I don't know 🤔

@tmadlener
Copy link
Contributor

I like the idea. We would probably then also need a pre-commit setup that checks in all repositories. In the end the targets are only defined but not run by default, so it would be a somewhat easy way for users to fix formatting issues that are flagged by pre-commit.

@m-fila
Copy link
Contributor Author

m-fila commented Sep 19, 2024

Hmm pre-commit config is just a single file and if we want to centrally manage it then the projects are not supposed to modify it, right?
So if we decide that we don't want for instance clang-tidy in pre-commit everywhere then adding clang-tidy just to selected repos would be a problem?
Similar problem with ruff config, ruff format and lint are in the same file so if we want to have managed ruff config with just format then it wont be possible to add lint to it. Also the config then should be probably moved from recommended pyproject.toml to ruff specific otherwise it won't be possible to modify pyproject.toml?

@tmadlener
Copy link
Contributor

I think some of the configuration files simply can't be managed centrally, or at least not for all packages. It's not entirely clear to me where to draw the line, but e.g. pyproject.toml is not something I would manage centrally, since there is a lot of project specific metadata that goes into there.

@m-fila
Copy link
Contributor Author

m-fila commented Sep 19, 2024

So should we centrally manage just the cmake target with ruff format but no linting target, pre-commit config, ruff config? Ruff can run on the defaults without any config file, the only difference between the defaults and the config here is the target-version = "py38" vs target-version = "py310"

@tmadlener
Copy link
Contributor

Maybe python related targets need to go into a separate .cmake file? I am not entirely sure if all the repositories will need it in the end. Additionally, Gaudi option files are not easily lintable, I think, since most of the things are dynamically populated.

I would leave the pre-commit config on a per repository basis at least. (The github action can then again be distributed centrally). For the linting, I am not entirely sure, in principle we should apply the same rules for all repositories. However, that would then mean that we would need to go the way of ruff specific config files, right? Because pyproject.toml is definitely repository specific, IMHO.

@tmadlener
Copy link
Contributor

Should we merge this still before the tag (tomorrow)? Or do we postpone this until later and conclude the discussion first?

@m-fila
Copy link
Contributor Author

m-fila commented Sep 19, 2024

I think there is no hurry for this one
I just realized I missed there is a black config in k4FWCore with different line width (99). I think I'll change it here to 99 too for some consistency

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks. If nobody complains, I will merge this later today.

@@ -0,0 +1,5 @@
target-version = "py310"
line-length = 99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just about to say we should add this. Nice :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@m-fila m-fila Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to be compatible with the black config from k4FWCore https://github.com/key4hep/k4FWCore/blob/main/pyproject.toml since at that moment that was the only python formatting config out there (except for spack I think)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's put py310 in k4geo (@tmadlener)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to py311 here and in k4FWCore and reformatting doesn't change any files. We have python 3.11 in the nightlies. I think we could just update and have py311 everywhere

I can also copy the extra formatting options from k4geo (it's just being explicitly setting the values same as the default right?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just being explicitly setting the values same as the default right?

Yes. Specifically the defaults that make ruff work / format the same as black.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could just update and have py311 everywhere

I would be in favor of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated here

@andresailer
Copy link
Collaborator

No ruff in the LCG releases? 👀

There will in the next one: https://lcginfo.cern.ch/pkg/ruff/

(Hopefully happening Monday)

@tmadlener
Copy link
Contributor

We could also switch over to use a key4hep stack, since that is what we are doing on other repositories, I think. Using an LCG stack was done historically, because we could more easily get an LLVM based stack (for clang-format), but that is now also available from a key4hep stack

@tmadlener tmadlener merged commit eb8592e into key4hep:main Sep 30, 2024
7 of 10 checks passed
@m-fila m-fila deleted the pre_commit_python branch September 30, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add python linters to pre-commit
4 participants